You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security review (PR 3793)
Outcome: No medium, high, or critical vulnerabilities were identified in the added or modified code after tracing attacker-controlled inputs to sinks and checking existing controls (parameterized SQL, project-scoped middleware, API field handling).
Prior threads: Previous automation findings were re-evaluated against the current diff; none warranted re-reporting with a concrete exploit path tied to this change set.
Notes (non-findings):
project__secret_sync inserts use bound parameters; path/prefix/separator values are not concatenated into SQL.
Manual SyncSecrets uses storage from middleware; sync config is loaded by storage_id with the same scope.
GetAppSysProcAttr applies optional Linux namespaces only to app runners; git continues to use GetSysProcAttr (per code comments and CmdGitClient).
Pro SyncSecrets is currently a stub returning nil; scheduler work is effectively no-op until implemented—treated as incomplete feature surface, not a demonstrated vulnerability in this PR.
The reason will be displayed to describe this comment to others. Learn more.
Stale comment
Security review (sync flag refactor)
Outcome: One high finding remains: secret storage deletion can leave orphaned secrets in external secret backends when sync had imported keys.
Not reported:SyncSecrets is a no-op in OSS/pro stub (no SSRF/injection from sync execution in this diff). SQL uses parameterized queries. Synchronized is forced server-side on create/update. Linux namespace flags are opt-in config.
Slack-ready summary: PR #3793 — 1 high: deleting a sync-enabled secret storage bulk-deletes imported keys with DeleteAccessKey only, skipping DeleteSecret / deserializer cleanup, so Vault/AWS SM/Azure KV entries can survive after the Semaphore row is gone.
The reason will be displayed to describe this comment to others. Learn more.
Security review (PR 3793)
Verdict: One high-severity issue remains: environment secret-sync persistence trusts client-supplied secret_storage_id without verifying the storage belongs to the same project as the environment, so a project member can point sync config at another project's secret storage (IDOR / cross-tenant data boundary). Other changes (parameterized SQL, sync stub, namespace split for app vs git, key synchronized hardening) did not surface additional high-confidence exploit paths in this diff.
Slack (copy/paste): PR 3793 — 1 High: cross-project secret_storage_id in saveEnvironmentSync lets env updates attach sync rows to arbitrary storage_id; fix by validating GetSecretStorage(env.ProjectID, *SecretStorageID) before SaveSecretSync. No other medium+ findings from this pass.
The reason will be displayed to describe this comment to others. Learn more.
High — IDOR / cross-project secret storage binding
saveEnvironmentSync copies env.SecretStorageID from the JSON body into sync.StorageID without checking that that storage row belongs to env.ProjectID. A user with access to project A can set secret_storage_id to a storage ID from project B (guessed or observed); the FK only constrains storage_id to exist somewhere in project__secret_storage, not to match the environment's project. project_id on the sync row stays A while storage_id points at B's vault — wrong data boundary and a plausible path to future sync/import logic operating on another tenant's backend once implemented.
Fix: Before persisting, load the storage with GetSecretStorage(env.ProjectID, *env.SecretStorageID) (or equivalent) and reject on ErrNotFound / mismatch.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fiftin
commented
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
